validate post GRT WNS opto#4320
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new optimization step, controlled by the OPT_POST_GRT_WNS variable, to optimize worst negative slack (WNS) after global routing using VT swapping and wire rerouting. Feedback on these changes highlights a redundant detailed placement and global routing run that occurs when this optimization is enabled, and recommends propagating the MATCH_CELL_FOOTPRINT and SETUP_SLACK_MARGIN parameters to the repair_timing command to ensure user configurations are respected.
|
@jhkim-pii, @jfgava, FYI. |
|
If the focus is just on WNS, we can try different phases, such as the "WNS" policy (-phases "WNS"). The problem is that it will focus only on the most critical path. If it can't be optimized, the run ends. There is no optimization on the secondary paths (no TNS gains). |
|
I agree that focusing on WNS rather than TNS is required to minimize the PNR impact. I also investigate how to fix the post-route timing violations effectively. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 106213bbaa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if { [env_var_exists_and_non_empty MATCH_CELL_FOOTPRINT] } { | ||
| lappend repair_timing_args -match_cell_footprint |
There was a problem hiding this comment.
Because defaults.py exports JSON defaults, MATCH_CELL_FOOTPRINT is present as the non-empty string 0 in normal flows that do not enable it. This condition therefore always appends -match_cell_footprint for the new post-GRT WNS repair pass, unlike repair_timing_helper/recover_power_helper which only add the flag when the variable equals 1; on libraries where useful VT swaps do not share a footprint, the default pass will be more constrained than requested and can miss the intended WNS improvement.
Useful? React with 👍 / 👎.
| # Route the modified nets by rsz journal restore | ||
| log_cmd global_route -end_incremental {*}$res_aware \ | ||
| -congestion_report_file $::env(REPORTS_DIR)/congestion_post_recover_power.rpt | ||
| if { !$::env(OPT_POST_GRT_WNS) } { |
There was a problem hiding this comment.
With OPT_POST_GRT_WNS defaulting to 1, this guard skips recover_power_helper in the default flow, so existing designs that set RECOVER_POWER (for example flow/designs/gf55/aes/config.mk and flow/designs/asap7/jpeg_lvt/config.mk) silently lose the post-global-route power recovery stage that previously ran whenever RECOVER_POWER was nonzero. The new WNS pass should not make the user-requested power recovery unreachable unless that incompatibility is explicitly handled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@precisionmoon this could be a cause of wns improvement as we know recover_power can sometimes damage it. We should fix this and retest.
Signed-off-by: Cho Moon <cmoon@precisioninno.com>
Signed-off-by: Cho Moon <cmoon@precisioninno.com> Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
Signed-off-by: Cho Moon <cmoon@precisioninno.com> Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
Signed-off-by: Cho Moon <cmoon@precisioninno.com> Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
Signed-off-by: Cho Moon <cmoon@precisioninno.com> Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
openroad-ci
force-pushed
the
secure-opt-grt-wns
branch
from
106213b to
3b129ed
Compare
5 participants
Timing benefits on private PDK designs seem quite significant.
Runtime impact seems small for hercules_is_int:
Base runtime (verific): 582 sec
Test runtime (verific): 575 sec